Add support for pointGroupingSchemes/groupingByLine - #21
Conversation
A structured point cloud may carry a per-scan-line index alongside its points: one group per row or column, saying where that line's points begin in the point vector and how many there are. A reader can use it to seek to a single scan line without decoding the whole cloud. This adds reading and writing of it. The groups are an ordinary compressed vector, so both directions reuse the machinery that already handles points: the writer lays down a second section during finalize, and the reader describes the section as a point cloud and hands it to PointCloudReaderRaw. `PointGroups::new` derives the declared limits from the groups, which is what you want when writing a cloud of your own. They stay public so they can be set explicitly, which is what you want when matching a file another implementation would have written -- not every writer out there declares the obvious maxima. Values outside their declared limits are refused rather than written, since they would be encoded in too few bits and silently lost. Only `groupingByLine` is supported; grouping schemes from extensions, such as the `las:groupingByReturnIndex` in the existing test file testdata/las2e57_no_images_tag.e57, are ignored rather than misparsed -- they do not necessarily have the same fields.
|
For convenience, since fork PRs need your approval before checks run here: the full CI matrix passes on my fork against this branch — Linux x86-64 and arm64, Windows x86-64 and arm64, macOS x86-64 and arm64, all six green, including clippy https://github.com/ohfaro/e57/actions/runs/31422975885 Prepared by GitHub Copilot on behalf of @ohfaro. |
|
Mhm, this is a difficult one. So far I intentionally did not try to implement the line grouping stuff because it feels to me like a weird feature that is not very useful in real world cases. The main advantage stated for this feature is fast "random-access" style reading for individual scan lines for structured scans without parsing the whole file. Even after this PR, this is not supported by the code and would require more complex changes. The same is also true for Andy Maloney’s widely used C++ libE57Format library, which does not implement the required seek for compressed vector reading: https://github.com/asmaloney/libE57Format/blob/master/src/CompressedVectorReaderImpl.cpp#L545 Also, the point grouping binary data and XML data reuses a lot of the methods for storing binary point data, but the PR does not reflect this as much as I would like to, especially in the XML part for the metadata. It feels like the LLM did find the solution that required the least amount of changes on the existing code. But I think a "proper" implementation needs to be more invasive. And as the maintainer I do not want to leave such changes to external contributors or LLMs, since I am the one that needs to maintain it in the end. Overall, I do not want to merge this code. Instead I would prefer smaller changes that allow client code to write/read the point grouping on its own, or my own implementation derived from this PR. |
A structured point cloud may carry a per-scan-line index alongside its points —
one group per row or column, saying where that line's points begin in the point
vector and how many there are. A reader can use it to seek to a single scan line
without decoding the whole cloud. The crate currently ignores it on read and
cannot produce it on write.
I ran into this writing files that have to match, byte for byte, what an
existing libE57Format-based writer produces: it emits
groupingByLinefor everystructured scan, so files written without it are not equivalent. It also can't
be added from outside the crate — the groups are a compressed vector, and the
packet, bytestream and paged-writer machinery is private.
finalize_customized_xmlcan add the XML but not the binary section it has to point at.
What this adds
New public types in
src/point_groups.rs:PointGroup,PointGroupLimits,PointGroups,PointGroupsHeader.Notes on the design
Both directions reuse what's already there. The groups are an ordinary
compressed vector, so the writer lays down a second section during
finalizeusing the same packet/bytestream path as points, and the reader describes the
section as a
PointCloudand hands it toPointCloudReaderRaw. No new formatcode.
Limits are derived but overridable.
PointGroups::newcomputes the declaredmaxima from the groups, which is what you want writing a cloud of your own. The
limitsfield stays public because matching another implementation sometimesmeans declaring something other than the obvious maximum — not every writer out
there gets this right, and reproducing a file faithfully means reproducing what
it declared. Happy to make this private if you'd rather keep the API narrow;
the derived path covers the common case on its own.
Out-of-range values are refused, not written. A value that doesn't fit the
limits it's declared under would be encoded in too few bits and silently lost.
libE57Format rejects this with
E57_ERROR_VALUE_OUT_OF_BOUNDS; this does too.Only
groupingByLineis supported. Extension schemes are ignored ratherthan misparsed — they don't necessarily have the same fields. Your existing
testdata/las2e57_no_images_tag.e57is a good example: it carrieslas:groupingByReturnIndex, which has nostartPointIndexat all. There's atest asserting that file still reads and reports no groups.
Tests
tests/writer_tests.rs— round trip of a 4x2 structured scan; a cloudwithout groups reporting
None; out-of-range values refused; a grouping keyedon something other than row/column refused.
tests/reader_tests.rs— the LAS extension scheme above is ignored.src/point_groups.rs— limit derivation, the prototype, and XML round trip.Full CI gate green locally on Linux x86-64:
cargo build --release --all,cargo test --release --all(102 tests, 0 failures),cargo clippy --release --all --all-targets --all-features -- -D warnings,cargo fmt --all -- --check,and
RUSTDOCFLAGS="-Dwarnings" cargo doc -p e57.Added an
## [Unreleased]CHANGELOG entry; move or reword it as you prefer.Prepared by GitHub Copilot on behalf of @ohfaro.